WP/AlternativeFunctions: add tests for namespaced names tests and fix handling of class functions/constants/properties #2617
Conversation
…ants/functions from non-WP class-based ones This commit changes two regexes used to identify global WP constants and functions to prevent the sniff from incorrectly identifying class constants/methods with the same names as WordPress globals as being WordPress globals. This is done by adding a negative lookbehind to ensure the searched strings are not preceded by an object operator, null-safe object operator, or scope resolution operator. Fixes part of 2603
| /* | ||
| * Safeguard correct handling of namespaced parameters passed to file_get_contents(). | ||
| * | ||
| * Note: the sniff should flag the examples using a fully qualified namespaced name and partially | ||
| * qualified name, but currently does not. This will be addressed via | ||
| * https://github.com/WordPress/WordPress-Coding-Standards/issues/2603. | ||
| */ | ||
| file_get_contents(\wp_upload_dir()['path'] . 'subdir/file.inc'); | ||
| file_get_contents(MyNamespace\wp_upload_dir()['path'] . 'subdir/file.inc'); | ||
| file_get_contents(\MyNamespace\wp_upload_dir()['path'] . 'subdir/file.inc'); | ||
| file_get_contents(namespace\wp_upload_dir()['path'] . 'subdir/file.inc'); | ||
| file_get_contents(\ABSPATH . 'wp-admin/css/some-file.css'); | ||
| file_get_contents(MyNamespace\ABSPATH . 'wp-admin/css/some-file.css'); | ||
| file_get_contents(\MyNamespace\ABSPATH . 'wp-admin/css/some-file.css'); | ||
| file_get_contents(namespace\ABSPATH . 'wp-admin/css/some-file.css'); |
There was a problem hiding this comment.
I wonder if it wouldn't be better to add these tests in the commit which addresses #2603 ? That way, the tests and the fix would be in the same commit (=atomic), making it more straight-forward in the future to understand the history of changes in the sniff.
There was a problem hiding this comment.
That was my original plan. Maybe I misunderstood what you said when we discussed this on Monday, but I thought you had suggested including those tests together with the rest of the namespaced names tests above. I'm happy to remove them from this PR and leave them for when #2603 is addressed.
| /* | ||
| * Safeguard that the sniff does not incorrectly ignore class methods/constants with the same | ||
| * name as WordPress global functions/constants when used in file_get_contents(). | ||
| */ |
There was a problem hiding this comment.
These tests cover the change, but might be good to vary the constants/functions being referenced in the tests ? And for the functions to include some case variation ?
There was a problem hiding this comment.
I'm unsure about this. I understand your point, but at the same time, to me, it is easier to understand the tests if there is no variation in parts of the code that are irrelevant to what is being tested. That is why I prefer to use the same constants/functions and all lowercase letters. There are already other tests that cover those variations for this specific part of the code.
There was a problem hiding this comment.
Yeah, but depending on the sniff, some functions may have special handling, while others do not. If all "namespaced name" tests only relate to the one function, it can easily be missed that special handling for another function is broken in the context of namespaced names (typically FQN global function).
I'm not trying to make your life more difficult, I'm trying to make it easier by having tests which will actually show what needs fixing for PHPCS 4.0.
There was a problem hiding this comment.
That is fair. I went ahead and added variation for the tests that safeguard that the sniff does not incorrectly ignore class methods/constants with the same name as WordPress global functions/constants when used in file_get_contents(). I did not do the same for the tests added in the other commit, as I imagine they will be removed from this PR pending what we decide in the other comment thread in this PR. I do plan to add variation to those tests as well.
… incorrectly ignore class methods/constants with the same name as WordPress global functions/constants when used in file_get_contents()
2 participants
Description
WP/AlternativeFunctions: add tests for namespaced names
Add tests for namespaced names, including some that currently are not flagged by the sniff but will be flagged once #2603 is fully addressed. As discussed in that issue, it will be easier to address this part once support for PHPCS 3.x is dropped.
WP/AlternativeFunctions: improve regex to distinguish global WP constants/functions from non-WP class-based ones
This commit changes two regexes used to identify global WP constants and functions to prevent the sniff from incorrectly identifying class constants/methods with the same names as WordPress globals as being WordPress globals. This is done by adding a negative lookbehind to ensure the searched strings are not preceded by an object operator, null-safe object operator, or scope resolution operator.
Suggested changelog entry
Fixed:
WordPress.WP.AlternativeFunctions: prevents false negatives when class functions/constants/properties use the same name as select global WP constants/functions.Related issues/external references
Partially addresses #2603